Fix #460: skip redundant agent-identity S2S grant when role is inherited#461
Merged
sellakumaran merged 3 commits intoJun 15, 2026
Merged
Conversation
When inheritable permissions are configured (allAllowed) and the blueprint SP holds the app roles, the agent identity inherits them automatically — the same basis the OBO/delegated path already relies on. The direct grant on the agent identity SP was therefore redundant and, when the CLI token cannot write app roles, produced a spurious "Action Required" PowerShell block on an otherwise successful Global Admin run. - Skip the per-identity grant (and the PowerShell prompt) when the blueprint grant and inheritance both succeeded; report the inherited grant as Granted. - When a direct grant is still needed (developer / non-inherited path), retry via az rest before falling back to PowerShell, matching the blueprint grant. - Correct stale README/CHANGELOG claim that Phase 2a/2b are always skipped for non-DW agents (untrue since #421). - Add review-time checks for comment-essay and CHANGELOG crispness (pr-code-reviewer #30/#31, review-pr SKILL) and codify both in CLAUDE.md and copilot-instructions. Verified live: setup completes cleanly with no PowerShell block, and query-entra reports the Observability app role granted on the blueprint SP with effective inheritance OK.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
github-actions
Bot
added
bug
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the non-DW setup flow to avoid redundant S2S app-role grants on the agent identity when those roles are already inherited from the blueprint, preventing unnecessary “Action Required” manual steps for Global Admin runs while preserving the developer/non-inherited path.
Changes:
- Skip the per-agent-identity S2S app-role assignment when Phase 2 inheritance is configured and the blueprint S2S grant succeeded; report the effective result as Granted.
- When a direct grant is still needed, retry S2S assignment via
az restbefore falling back to PowerShell instructions. - Refresh setup documentation/release notes and codify review-time standards for crisp comments and release-note-ready CHANGELOG entries.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/NonDwBlueprintSetupOrchestrator.cs | Adds inheritance gate to skip redundant S2S grant; adds az rest retry before PowerShell fallback. |
| src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/NonDwBlueprintSetupOrchestratorExecuteTests.cs | Expands test coverage for az rest fallback and inheritance-based skip behavior. |
| src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/README.md | Updates non-DW setup flow docs to reflect inheritance + fallback behavior. |
| CHANGELOG.md | Corrects stale non-DW Phase 2 wording; adds fix entry for issue #460 behavior. |
| CLAUDE.md | Codifies repo guidance for crisp code comments and consumer-facing CHANGELOG entries. |
| .github/copilot-instructions.md | Mirrors the crisp-comment and CHANGELOG guidance for Copilot usage. |
| .claude/skills/review-pr/SKILL.md | Updates review-pr skill principles to enforce crisp comments and CHANGELOG quality. |
| .claude/agents/pr-code-reviewer.md | Adds explicit reviewer checks for “comment essays” and non-release-note-ready CHANGELOG entries. |
… GET stub in test - Collapse multi-line XML <summary>/inline comment blocks to a one-line why plus issue ref (pr-code-reviewer #30). - Stub the initial GET appRoleAssignments call explicitly in the Graph-and-az-rest-both-fail test so it no longer relies on BuildMockExecutor's default.
…terminism rule - pr-code-reviewer #30: mandatory-emit so the comment-density sweep is reported every run, not dropped as low-severity noise. - pr-code-reviewer #32: flag a test that relies on a shared mock-builder default instead of stubbing the call explicitly. - review-pr SKILL: read the working-tree copy of the rule files so a PR that adds a review rule is checked by that rule in the same run.
sruthikilari
approved these changes
Jun 15, 2026
gwharris7
approved these changes
Jun 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #460
When inheritable permissions are configured (allAllowed) and the blueprint SP holds the app roles, the agent identity inherits them automatically — the same basis the OBO/delegated path already relies on. The direct grant on the agent identity SP was therefore redundant and, when the CLI token cannot write app roles, produced a spurious "Action Required" PowerShell block on an otherwise successful Global Admin run.
Verified live: setup completes cleanly with no PowerShell block, and query-entra reports the Observability app role granted on the blueprint SP with effective inheritance OK.